-
Notifications
You must be signed in to change notification settings - Fork 153
feat: Add max query payment support to Query #1347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1347 +/- ##
==========================================
+ Coverage 92.54% 92.58% +0.03%
==========================================
Files 139 139
Lines 8559 8587 +28
==========================================
+ Hits 7921 7950 +29
+ Misses 638 637 -1 🚀 New features to boost your workflow:
|
|
Hi, this is WorkflowBot.
|
aceppaluni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manishdait , this is looking great!
The new default max_query_payment (1 HBAR) is correctly enforced, but it’s causing the Run Examples workflow to fail.
In examples/contract/ethereum_transaction.py, a ContractCallQuery is executed without setting max_query_payment. The reported query cost (~1.42 HBAR) exceeds the new default, so _before_execute() raises as expected.
Other examples and integration tests in this PR already follow the new pattern:
cost = query.get_cost(client)
query.set_max_query_payment(cost)
result = query.execute(client)
What do you think of updating the Ethereum transaction example to explicitly set max_query_payment in the same way?
WalkthroughThe PR adds maximum query payment limits to the Python SDK. It introduces per-query Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/contract/ethereum_transaction.py (1)
149-164: Consider simplifying the cost-fetch-and-set pattern for better UX.This is the third example file with the same cost-fetch-and-set pattern. While consistent across examples, the pattern remains redundant—fetching the cost just to set
max_query_paymentto that exact value doesn't effectively demonstrate protection against high query fees.For better user experience and clearer demonstration of the feature, consider:
Option 1: Set client default once in
setup_client():client.set_default_max_query_payment(Hbar(10))Option 2: Set a fixed max per query without fetching:
query = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) ) result = query.execute(client)If the cost-fetch pattern is intentional to guarantee execution success, consider adding a comment explaining why this approach is used rather than setting a client default.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/ethereum_transaction.pyexamples/contract/contract_execute_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/contract_call_query_e2e_test.py
🧬 Code graph analysis (8)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (4)
Client(32-287)from_env(59-103)for_testnet(106-115)set_default_max_query_payment(249-274)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/contract_call_query.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
🪛 Ruff (0.14.10)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (12)
CHANGELOG.md (1)
81-81: LGTM! Clear and accurate changelog entry.The changelog entry properly documents both the per-query and client-wide maximum payment configuration methods introduced in this PR.
tests/unit/client_test.py (1)
191-232: Excellent test coverage for set_default_max_query_payment!The test suite comprehensively covers:
- ✅ Valid input types (int, float, Decimal, Hbar) with correct conversion
- ✅ Default value verification (1 Hbar)
- ✅ Negative values raising ValueError with descriptive messages
- ✅ Invalid types raising TypeError with descriptive messages
The tests follow unit testing best practices and will effectively protect against breaking changes.
tests/integration/contract_call_query_e2e_test.py (1)
132-179: LGTM: test validates get_cost() with manual payment setting.This test appropriately uses
set_query_payment()to manually override the payment rather thanset_max_query_payment(), which is the correct pattern for validating cost calculation with explicit payment amounts.src/hiero_sdk_python/query/query.py (2)
5-5: LGTM: Import additions support new feature.The
Decimalimport is necessary for the type validation inset_max_query_payment(), and the import reorganization is clean.Also applies to: 22-22
96-124: LGTM: Robust input validation and conversion.The method correctly validates input types, handles negative values, and converts numeric types to
Hbar. The fluent interface (returningself) is properly maintained.Static analysis hints about long error messages (TRY003) and Union syntax (UP007) are stylistic preferences and don't impact functionality.
src/hiero_sdk_python/client/client.py (3)
5-5: LGTM: Well-defined constant with appropriate default.The
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a sensible baseline for query cost protection. Imports support the type validation logic.Also applies to: 11-11, 23-24
52-52: LGTM: Proper initialization from module constant.Instance variable allows per-client customization while maintaining a sensible default.
249-274: LGTM: Validation logic mirrors Query.set_max_query_payment.The implementation correctly validates input types and values, matching the pattern in
Query.set_max_query_payment(). The code duplication is acceptable given it only appears in two locations and centralizing it might reduce clarity.Static analysis hints (TRY003, UP007) are stylistic and don't impact functionality.
tests/unit/query_test.py (4)
1-2: LGTM: Necessary imports for comprehensive test coverage.
Decimalenables testing ofDecimalinput types, andreis needed for precise error message matching with special characters (ℏ).
60-60: LGTM: Test updated to accommodate new validation.Setting
max_query_paymentto 3 Hbar when the mocked cost is 2 Hbar ensures the payment validation passes, which is the expected behavior for this test.
253-289: Excellent coverage of input validation scenarios.The parameterized tests comprehensively validate:
- Type conversions (int, float, Decimal, Hbar)
- Zero and positive values
- Negative value rejection
- Invalid type rejection
Error message matching correctly uses type names and values.
Minor style note: Static analysis suggests using a tuple for the first argument of
@pytest.mark.parametrizeon line 254 (e.g.,('valid_amount', 'expected')instead of'valid_amount,expected'), but this is cosmetic and both forms work.
291-376: Comprehensive test coverage for query/client max payment interactions.These tests thoroughly validate:
- Query-specific
max_query_paymentoverriding client default- Fallback to
client.default_max_query_paymentwhen not set- Both success scenarios (cost ≤ max) and failure scenarios (cost > max)
- Correct error message formatting
The mocked cost values ensure deterministic test behavior, and the test names clearly describe the scenarios being validated.
44b253e to
c773357
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.py
🧬 Code graph analysis (8)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (2)
Client(32-287)set_default_max_query_payment(249-274)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (5)
ContractCallQuery(27-233)set_contract_id(59-67)set_gas(69-77)set_function(107-124)execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/integration/query_e2e_test.py (2)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/integration/query_e2e_test.py
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (12)
src/hiero_sdk_python/client/client.py (2)
23-24: LGTM! Reasonable default for maximum query payment.The
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)constant provides a sensible client-wide default that protects users from unexpectedly high query fees while allowing sufficient headroom for typical queries.
249-274: LGTM! Robust implementation with proper validation.The
set_default_max_query_payment()method correctly:
- Validates input types and rejects invalid types with clear error messages
- Ensures non-negative values for numeric inputs
- Converts numeric types to
Hbarobjects consistently- Returns
selffor method chainingThe static analysis hints about Union syntax and exception message style are minor preferences for Python 3.10+ and do not affect functionality.
examples/contract/contract_execute_transaction.py (1)
110-124: [Duplicate] Reconsider the cost-fetch-and-set pattern for better user guidance.As noted in previous review feedback, the current pattern (fetch cost → set max to exact cost → execute) is redundant and doesn't effectively demonstrate the protection feature. Setting
max_query_paymentto exactly the fetched cost will always pass, which defeats the purpose of showing users how to protect against unexpectedly high fees.Consider one of these simpler, more instructive alternatives:
Option 1 (Recommended): Set a client-wide default once in
setup_client():def setup_client(): """Initialize and set up the client with operator account""" network = Network(network_name) print(f"Connecting to Hedera {network_name} network!") client = Client(network) operator_id = AccountId.from_string(os.getenv("OPERATOR_ID", "")) operator_key = PrivateKey.from_string(os.getenv("OPERATOR_KEY", "")) client.set_operator(operator_id, operator_key) client.set_default_max_query_payment(Hbar(10)) # Set once for all queries print(f"Client set up with operator id {client.operator_account_id}") return clientThen queries execute directly without the cost-fetch step:
def get_contract_message(client, contract_id): """Get the message from the contract""" result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .execute(client) ) return result.get_bytes32(0).decode("utf-8")Option 2: Set a fixed reasonable max per query:
def get_contract_message(client, contract_id): """Get the message from the contract""" result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) .execute(client) ) return result.get_bytes32(0).decode("utf-8")Both approaches are simpler for users to copy-paste and better demonstrate the protection feature.
Based on past review comments and examples coding guidelines.
Likely an incorrect or invalid review comment.
examples/contract/ethereum_transaction.py (1)
146-164: LGTM! Correctly demonstrates the two-step cost-aware query pattern.The updated
get_contract_messagefunction properly demonstrates the new workflow:
- Construct the query
- Fetch the cost with
get_cost(client)- Set the max payment with
set_max_query_payment(cost)- Execute the query
This provides a clear example for SDK users on how to use the new max query payment feature.
tests/integration/contract_call_query_e2e_test.py (1)
116-127: LGTM! Two-step pattern correctly applied.The test properly demonstrates fetching the cost before setting the max payment and executing.
src/hiero_sdk_python/query/query.py (2)
143-157: Max payment check only applies to auto-fetched costs.The max payment validation (lines 145-157) only triggers when
self.payment_amount is Noneand is set viaget_cost(). If a user manually setspayment_amountviaset_query_payment(), the max check is bypassed entirely.Is this intentional? If a user does:
query.set_max_query_payment(Hbar(1)) query.set_query_payment(Hbar(5)) # Exceeds max, but no validation query.execute(client)The query will execute without the max payment protection. Consider whether the max check should also validate manually-set payments, or document this behavior explicitly.
96-124: LGTM! Setter implementation is correct.The
set_max_query_paymentmethod correctly:
- Validates input types (int, float, Decimal, Hbar)
- Rejects negative values with a clear error message
- Converts non-Hbar inputs to Hbar
- Returns
selffor method chainingtests/unit/query_test.py (5)
328-335: Duplicate assignment removed - LGTM.The previously flagged duplicate assignment (
query_requires_payment.get_cost = mock_get_cost) appears to have been resolved. The test correctly validates that execution fails when cost exceeds the query-specific max.
366-373: Duplicate assignment removed - LGTM.The previously flagged duplicate assignment has been resolved. The test correctly validates that execution fails when cost exceeds the client's default max when no query override exists.
291-310: LGTM! Comprehensive test for query max overriding client default.The test correctly validates:
- Client has default max of 1 Hbar
- Query overrides with 2 Hbar
- Cost of 2 Hbar succeeds because it's within query's max
- Payment amount is correctly set
338-357: LGTM! Tests client default max fallback behavior.The test correctly validates that when no query-level max is set, the client's default is used, and execution succeeds when cost is within that limit.
251-251: Avoid equality comparison toTrue.Use the truthiness check directly instead of comparing to
True.🔎 Proposed fix
- assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()Likely an incorrect or invalid review comment.
| def test_set_default_max_query_payment_valid_param(valid_amount, expected): | ||
| """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" | ||
| client = Client.for_testnet() | ||
| # by default is 1 hbar before setting it | ||
| assert client.default_max_query_payment == Hbar(1) | ||
| client.set_default_max_query_payment(valid_amount) | ||
| assert client.default_max_query_payment == expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Add test for fluent interface (method chaining).
Per coding guidelines, fluent setters should be tested to return self. The test verifies value conversion but doesn't assert that set_default_max_query_payment returns the client instance for method chaining.
🔎 Proposed fix
def test_set_default_max_query_payment_valid_param(valid_amount, expected):
"""Test that set_default_max_query_payment correctly converts various input types to Hbar."""
client = Client.for_testnet()
# by default is 1 hbar before setting it
assert client.default_max_query_payment == Hbar(1)
- client.set_default_max_query_payment(valid_amount)
+ result = client.set_default_max_query_payment(valid_amount)
+ assert result is client, "set_default_max_query_payment should return self for method chaining"
assert client.default_max_query_payment == expected
+ client.close()As per coding guidelines: "Assert fluent setters return self".
| def test_set_default_max_query_payment_negative_value(negative_amount): | ||
| """Test set_default_max_query_payment for negative amount values.""" | ||
| client = Client.for_testnet() | ||
|
|
||
| with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"): | ||
| client.set_default_max_query_payment(negative_amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Add resource cleanup in negative value tests.
The test creates a client but doesn't close it. Consider adding cleanup for consistency with other tests in this file.
🔎 Proposed fix
def test_set_default_max_query_payment_negative_value(negative_amount):
"""Test set_default_max_query_payment for negative amount values."""
client = Client.for_testnet()
with pytest.raises(ValueError, match=f"max_query_payment must be non-negative, got {negative_amount}"):
client.set_default_max_query_payment(negative_amount)
+
+ client.close()c773357 to
8ee657c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/integration/query_e2e_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.py
🧬 Code graph analysis (2)
tests/integration/query_e2e_test.py (3)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)tests/integration/utils.py (2)
IntegrationTestEnv(34-96)env(23-27)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
tests/unit/query_test.py (3)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
🪛 Ruff (0.14.10)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (1)
tests/unit/query_test.py (1)
291-374: Good test coverage for max_query_payment behavior.The tests comprehensively cover:
- Query overriding client default max payment (success case)
- Query override with cost exceeding query-specific max (error case)
- Query using client default max payment (success case)
- Query using client default with cost exceeding default (error case)
The use of
re.escape()for matching error messages with special characters (ℏ symbol) is appropriate.
tests/unit/query_test.py
Outdated
| query_requires_payment.set_max_query_payment(2) | ||
| assert query_requires_payment.max_query_payment == Hbar(2) | ||
|
|
||
| # mock the get_cost to resturn 2 hbar as required paymnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Fix typos in comments.
Minor typos in docstring comments.
🔎 Proposed fix
- # mock the get_cost to resturn 2 hbar as required paymnet
+ # mock the get_cost to return 2 hbar as required paymentApply at both lines 302 and 349.
Also applies to: 349-349
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
@manishdait This is looking great 👍 Please be sure to address merge conflicts. Thank you!! |
8ee657c to
7b9b950
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/ethereum_transaction.pyexamples/contract/contract_execute_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.pytests/integration/contract_call_query_e2e_test.py
🧬 Code graph analysis (7)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (2)
Client(32-287)set_default_max_query_payment(249-274)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/integration/contract_call_query_e2e_test.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (2)
CHANGELOG.md (1)
90-90: LGTM!The changelog entry clearly documents the new max query payment feature, including both per-query and client-wide default maximum payment methods.
examples/contract/contract_execute_transaction.py (1)
110-120: Reconsider the cost-fetch-and-set pattern for better user guidance.Same concern as in
contract_call_query.py: the current pattern (fetch cost → set max to exact cost → execute) doesn't effectively demonstrate the protection feature and adds unnecessary complexity for users.Consider using one of these simpler alternatives:
Option 1 (Recommended): Set a client-wide default once in
setup_client():def setup_client(): # ... existing setup code ... client.set_default_max_query_payment(Hbar(10)) # Set once for all queries return clientThen execute directly:
result = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .execute(client) )Option 2: Set a fixed reasonable max per query:
query = ( ContractCallQuery() .set_contract_id(contract_id) .set_gas(2000000) .set_function("getMessage") .set_max_query_payment(Hbar(5)) ) result = query.execute(client)Both approaches are simpler and better demonstrate how the protection feature works.
Based on examples coding guidelines.
Likely an incorrect or invalid review comment.
7b9b950 to
b309ddb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (3)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pyexamples/contract/contract_call_query.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.pytests/integration/contract_call_query_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:28:14.391Z
Learnt from: github-actions[bot]
Repo: hiero-ledger/hiero-sdk-python PR: 0
File: :0-0
Timestamp: 2026-01-08T10:28:14.391Z
Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
Applied to files:
tests/unit/client_test.pyexamples/contract/contract_call_query.py
🧬 Code graph analysis (8)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/integration/query_e2e_test.py (3)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)tests/integration/utils.py (3)
IntegrationTestEnv(34-96)env(23-27)close(51-52)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
tests/integration/contract_call_query_e2e_test.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
🪛 Ruff (0.14.10)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (20)
CHANGELOG.md (1)
93-93: LGTM!The changelog entry clearly documents both the per-query
set_max_query_payment()method and the client-wideset_default_max_query_payment()method. The entry is concise and properly placed in the "Added" section.tests/integration/contract_call_query_e2e_test.py (2)
66-77: LGTM! Clear demonstration of the new cost-aware query pattern.The test now follows the recommended two-step flow: construct the query, fetch the cost, set the maximum payment, and then execute. This pattern validates that the new
set_max_query_payment()API works correctly in an end-to-end scenario with a real network.
116-127: LGTM! Consistent with the first test.This test follows the same cost-aware query pattern as
test_integration_contract_call_query_can_execute_with_constructor, ensuring consistency across integration tests and demonstrating proper usage of the newset_max_query_payment()API.src/hiero_sdk_python/client/client.py (3)
23-23: LGTM! Sensible default value.The
DEFAULT_MAX_QUERY_PAYMENTconstant of 1 HBAR provides a reasonable baseline for query costs while protecting against unexpectedly expensive queries. This aligns with the PR objectives.
52-52: LGTM! Proper attribute initialization.The
default_max_query_paymentattribute is correctly initialized with theDEFAULT_MAX_QUERY_PAYMENTconstant in the__init__method. The type annotation is clear and consistent.
249-274: LGTM! Well-implemented with consistent validation.The
set_default_max_query_payment()method provides robust validation and follows the same pattern asQuery.set_max_query_payment(). The docstring clearly explains the relationship between the client-wide default and per-query overrides. The method chaining support is a good touch.examples/contract/contract_execute_transaction.py (1)
110-120: LGTM! Clear demonstration of the cost-aware query pattern.The example now demonstrates the recommended two-step flow for contract queries: construct the query, fetch the cost, set the maximum payment, and then execute. This pattern is clear for users who copy-paste examples and aligns with the new
max_query_paymentfeature.examples/contract/contract_call_query.py (1)
117-128: LGTM! Consistent with other contract examples.The example demonstrates the cost-aware query pattern correctly: construct the query, fetch the cost, set the maximum payment, and execute. This is consistent with the pattern used in
contract_execute_transaction.pyand provides clear guidance for users.tests/unit/client_test.py (3)
1-3: LGTM!The updated docstring accurately reflects the broader scope of tests, including the new
set_default_max_query_paymenttests.
209-218: LGTM!The test correctly validates that negative values raise
ValueErrorwith a descriptive message across multiple numeric types.
220-232: LGTM!The test correctly validates that invalid parameter types raise
TypeErrorwith a descriptive message.src/hiero_sdk_python/query/query.py (4)
5-7: LGTM!The
Decimalimport and updated type hints are necessary for the newmax_query_paymentfunctionality.
60-60: LGTM!The
max_query_paymentattribute is correctly initialized toNone, allowing optional per-query overrides.
96-124: LGTM!The
set_max_query_paymentmethod correctly:
- Validates input types (int, float, Decimal, Hbar)
- Checks non-negativity before conversion
- Normalizes to Hbar for consistent internal representation
- Returns self for method chaining
- Provides descriptive error messages
The implementation is consistent with
Client.set_default_max_query_paymentand follows SDK patterns.
146-157: LGTM!The max payment validation logic correctly:
- Determines the effective maximum: per-query override takes precedence, falls back to client default
- Compares actual cost against the maximum
- Raises a descriptive
ValueErrorwith both values in HBAR for clarityThe implementation aligns with the PR objectives to validate costs before query execution.
tests/unit/query_test.py (4)
1-2: LGTM!The
Decimalandreimports are necessary for the new parametrized tests and error message assertions.
54-76: LGTM!The test correctly updates to accommodate the new max payment validation by setting a sufficient limit (3 HBAR) for the mocked cost (2 HBAR).
270-277: LGTM!The test correctly validates that negative values raise
ValueErrorwith a descriptive message.
279-289: LGTM!The test correctly validates that invalid parameter types raise
TypeErrorwith descriptive messages.tests/integration/query_e2e_test.py (1)
9-12: LGTM!The new imports are necessary for the integration test validating max payment behavior.
|
Hi @manishdait |
b309ddb to
91c1706
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/hiero_sdk_python/query/query.py (2)
56-61: Reject negativeHbarvalues inset_max_query_payment()(currently only non-Hbar negatives are blocked).Proposed fix
def set_max_query_payment(self, max_query_payment: Union[int, float, Decimal, Hbar]) -> "Query": @@ - if not isinstance(max_query_payment, Hbar): - if max_query_payment < 0: - raise ValueError(f"max_query_payment must be non-negative, got {max_query_payment}") - - max_query_payment = Hbar(max_query_payment) + if isinstance(max_query_payment, Hbar): + if max_query_payment < Hbar(0): + raise ValueError("max_query_payment must be non-negative") + else: + if max_query_payment < 0: + raise ValueError(f"max_query_payment must be non-negative, got {max_query_payment}") + max_query_payment = Hbar(max_query_payment)Also applies to: 96-124
126-158: Max-payment enforcement is easy to bypass; align behavior and docstrings.Right now the max check only runs when
payment_amountisNone. If a caller precomputes cost and usesset_query_payment(cost)(a common pattern to avoid extra COST_ANSWER calls),max_query_paymentwon’t be enforced at all—despite theset_max_query_payment()docstring implying a pre-execution cost check.Low-risk enforcement (enforce only when a per-query max is explicitly set)
# If no payment amount was specified and payment is required for this query, # get the cost from the network and set it as the payment amount - if self.payment_amount is None and self._is_payment_required(): - self.payment_amount = self.get_cost(client) - - # if max_query_payment not set fall back to the client-level default max query payment - max_payment = ( - self.max_query_payment - if self.max_query_payment is not None - else client.default_max_query_payment - ) - - if self.payment_amount > max_payment: - raise ValueError( - f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR " - f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" - ) + if not self._is_payment_required(): + return + + # Effective max (per-query override, else client default) + max_payment = ( + self.max_query_payment + if self.max_query_payment is not None + else client.default_max_query_payment + ) + + # If caller explicitly set a per-query max, ensure an explicitly-set payment can't bypass it. + if self.max_query_payment is not None and self.payment_amount is not None: + if self.payment_amount > max_payment: + raise ValueError( + f"Query payment {self.payment_amount} exceeds max query payment {max_payment}" + ) + return + + if self.payment_amount is None: + self.payment_amount = self.get_cost(client) + if self.payment_amount > max_payment: + raise ValueError( + f"Query cost {self.payment_amount} exceeds max query payment {max_payment}" + )tests/unit/query_test.py (2)
246-252: Avoid== Trueassertions (ruff E712 / clarity).Use
assert query._is_payment_required() is True(or simplyassert query._is_payment_required()).
54-77: Good coverage of client-vs-query max payment behavior; add one fluent-return assert.Nice: the override vs client-default scenarios (and regex-escaped message matching) cover the critical behavior well. I’d still add a quick
is queryassertion forset_max_query_payment()in at least one “non-param” path (e.g., Line 299 / Line 325) to protect the fluent interface contract.Also applies to: 291-374
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (4)
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/contract_call_query.pyexamples/contract/ethereum_transaction.pyexamples/contract/contract_execute_transaction.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/query_e2e_test.pytests/integration/contract_call_query_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.pytests/unit/client_test.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:28:14.391Z
Learnt from: github-actions[bot]
Repo: hiero-ledger/hiero-sdk-python PR: 0
File: :0-0
Timestamp: 2026-01-08T10:28:14.391Z
Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
Applied to files:
examples/contract/contract_call_query.pytests/unit/client_test.py
🧬 Code graph analysis (9)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/ethereum_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
tests/integration/query_e2e_test.py (3)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)tests/integration/utils.py (4)
IntegrationTestEnv(34-96)env(23-27)close(51-52)create_account(54-67)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-124)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-124)_is_payment_required(414-421)get_cost(281-321)_before_execute(126-157)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-274)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)src/hiero_sdk_python/client/client.py (2)
Client(32-287)set_default_max_query_payment(249-274)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (5)
ContractCallQuery(27-233)set_contract_id(59-67)set_gas(69-77)set_function(107-124)execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(281-321)set_max_query_payment(96-124)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
154-157: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
tests/unit/query_test.py
251-251: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
254-254: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (1)
tests/integration/query_e2e_test.py (1)
9-13: Tighten new integration test + avoidenvname collision (ruff F811/PT011/PT018).
- This module now imports the
envfixture (Line 12) and other tests in the same file assignenv = IntegrationTestEnv(), which can trigger ruff F811—rename those locals (e.g.,itest_env) or switch them to accept theenvfixture parameter.- Make the new failure assertion deterministic with
match=and split the combined assert for clearer failure output.Proposed fix (new test only)
+import re ... def test_integration_query_exceeds_max_payment(env): """Test that Query fails when cost exceeds max_query_payment.""" - receipt = env.create_account(1) - account_id = receipt.id + account = env.create_account(1) + account_id = account.id ... - with pytest.raises(ValueError) as e: + with pytest.raises(ValueError, match=re.escape("exceeds max set query payment:")) as e: query.execute(env.client) msg = str(e.value) - assert "Query cost" in msg and "exceeds max set query payment:" in msg + assert "Query cost" in msg + assert "exceeds max set query payment:" in msgAlso applies to: 242-260
⛔ Skipped due to learnings
Learnt from: github-actions[bot] Repo: hiero-ledger/hiero-sdk-python PR: 0 File: :0-0 Timestamp: 2026-01-08T10:28:14.391Z Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/query_test.py (1)
248-253: Use truthiness check instead of equality comparison toTrue.Per Python best practices, use
assert query_requires_payment._is_payment_required()instead of comparing toTrue.🔧 Suggested fix
def test_query_payment_requirement_defaults_to_true(query_requires_payment): """Test that the base Query class and payment-requiring queries default to requiring payment.""" query = Query() - assert query._is_payment_required() == True + assert query._is_payment_required() # Verify that payment-requiring query also defaults to requiring payment - assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.py
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
🧬 Code graph analysis (3)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-128)_is_payment_required(418-425)get_cost(285-325)_before_execute(130-161)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-278)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
🪛 Ruff (0.14.10)
tests/unit/query_test.py
253-253: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
283-283: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
158-161: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (11)
src/hiero_sdk_python/query/query.py (3)
60-60: LGTM! Newmax_query_paymentattribute correctly initialized.The attribute is properly typed as
Optional[Hbar]and defaults toNone, allowing the fallback toclient.default_max_query_paymentin_before_execute.
96-128: LGTM! Well-implemented setter with proper validation.The method correctly:
- Validates input types (int, float, Decimal, Hbar)
- Enforces non-negativity for both Hbar and numeric inputs
- Normalizes to Hbar for consistent internal representation
- Returns
selffor method chaining
147-162: Payment validation logic is correct and preserves existing semantics.The max payment validation:
- Only applies to paid queries that haven't had
payment_amountmanually set- Correctly falls back to
client.default_max_query_paymentwhenmax_query_paymentisNone- Raises a descriptive error before query submission when cost exceeds the limit
This aligns with the PR objective to "fail early with a descriptive error."
src/hiero_sdk_python/client/client.py (3)
23-24: LGTM! Sensible default maximum query payment.
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a reasonable default that protects users from unexpectedly high query costs while being sufficient for most queries.
52-52: LGTM! Client attribute correctly initialized.The
default_max_query_paymentattribute is properly typed asHbar(notOptional) since there's always a sensible default value.
249-278: Correctly implemented setter with proper validation.The method correctly validates input types, enforces non-negativity, and returns
selffor method chaining. The implementation mirrorsQuery.set_max_query_payment, which ensures consistent behavior across the SDK.tests/unit/query_test.py (5)
1-15: LGTM! Appropriate imports and test categorization.The
Decimalandreimports support the new parametrized tests, andpytestmark = pytest.mark.unitproperly categorizes all tests in this file.
56-78: Correctly updated test to accommodate new validation.Setting
max_query_paymentto 3 Hbar prevents the new validation from failing when the mocked cost is 2 Hbar. The test continues to verify thatpayment_amountis correctly set fromget_cost().
272-279: LGTM! Comprehensive negative value testing.The parametrized test covers negative integers, floats, and Decimals with appropriate error message matching.
286-296: LGTM! Good invalid type coverage.The test covers strings,
None, and arbitrary objects, ensuringTypeErroris raised with a descriptive message.
298-380: Excellent integration test coverage for max payment interactions.The tests comprehensively cover:
- Query-specific max overriding client default (success path)
- Query-specific max exceeded (error path)
- Client default used when no query override (success path)
- Client default exceeded (error path)
Using
re.escape()for the error message match is correct given the specialℏcharacter.
| @pytest.mark.parametrize( | ||
| 'valid_amount,expected', | ||
| [ | ||
| (1, Hbar(1)), | ||
| (0.1, Hbar(0.1)), | ||
| (Decimal('0.1'), Hbar(Decimal('0.1'))), | ||
| (Hbar(1), Hbar(1)), | ||
| (Hbar(0), Hbar(0)) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider using a tuple for parametrize argument names.
Per pytest conventions, the first argument to @pytest.mark.parametrize is typically a tuple when multiple parameters are used.
♻️ Suggested fix
@pytest.mark.parametrize(
- 'valid_amount,expected',
+ ('valid_amount', 'expected'),
[
(1, Hbar(1)),
(0.1, Hbar(0.1)),
(Decimal('0.1'), Hbar(Decimal('0.1'))),
(Hbar(1), Hbar(1)),
(Hbar(0), Hbar(0))
]
)🧰 Tools
🪛 Ruff (0.14.10)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
666016b to
1f4667b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/hiero_sdk_python/query/query.py (1)
145-163: Enforce max cap even whenpayment_amountis explicitly set (currently bypasses the limit).
As written, a caller can doset_query_payment(Hbar(1000))and_before_execute()will not validate againstmax_query_payment/client.default_max_query_payment, which defeats the “max payment” safety guarantee.Proposed fix
def _before_execute(self, client: Client) -> None: @@ - if self.payment_amount is None and self._is_payment_required(): - self.payment_amount = self.get_cost(client) - - # if max_query_payment not set fall back to the client-level default max query payment - max_payment = ( - self.max_query_payment - if self.max_query_payment is not None - else client.default_max_query_payment - ) - - if self.payment_amount > max_payment: - raise ValueError( - f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR " - f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" - ) + if self._is_payment_required(): + # If max_query_payment not set, fall back to the client-level default max query payment + max_payment = self.max_query_payment if self.max_query_payment is not None else client.default_max_query_payment + + # Determine what we will actually pay. + payment_amount = self.payment_amount + if payment_amount is None: + payment_amount = self.get_cost(client) + self.payment_amount = payment_amount + + if payment_amount > max_payment: + raise ValueError( + f"Query cost ℏ{payment_amount.to_hbars()} HBAR " + f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" + )tests/unit/query_test.py (2)
248-254: Ruff cleanups likely needed (E712 / PT006).
Fixassert ... == True→assert ... is True(or justassert ...) and adjustpytest.mark.parametrizeargnames format per the project’s ruff settings, if CI enforces it.Also applies to: 255-265
56-78: Add a test for the manual payment amount path where the cap should be enforced.Currently, the suite validates cap enforcement only when
_before_execute()fetches cost viaget_cost(). Add a test: setquery_requires_payment.set_query_payment(Hbar(2)), setquery_requires_payment.set_max_query_payment(Hbar(1))(or lower the client default), then call_before_execute(mock_client)and assert it raisesValueError. This test protects against silently bypassing the max cap for manually set payments and ensures consistent validation behavior.src/hiero_sdk_python/client/client.py (1)
23-53: Confirm this is intended as a backward-incompatible breaking change and add to CHANGELOG.The 1 Hbar default for
default_max_query_paymentwill cause queries exceeding this cost to fail by default (as tests demonstrate: queries costing 2 Hbar fail with the default 1 Hbar limit). This needs explicit release notes documenting the behavior change for users upgrading.Regarding the shared instance:
Hbaris immutable (read-only value object with no setters), so assigning the same module-levelDEFAULT_MAX_QUERY_PAYMENTreference to each client is safe—creating fresh instances per client would be defensive but unnecessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.py
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
🧬 Code graph analysis (3)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(18-213)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-128)_is_payment_required(418-425)get_cost(285-325)_before_execute(130-161)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-278)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(18-213)to_hbars(67-71)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-265: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/query_test.py
253-253: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
158-161: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
aceppaluni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manishdait
This looks great overall!! 👍
In client.py, One thing I wanted to double-check. Since bool is a subclass of int in Python, this currently allows True / False to be passed and converted into Hbar(1) / Hbar(0).
Similarly, float("nan") and float("inf") would pass the type check and get passed into Hbar(...).
Do you think we should explicitly reject booleans and non-finite numbers here, or is that something we should expect Hbar to handle internally?
|
@aceppaluni You’re correct — because bool is a subclass of int, currently True and False would be accepted and converted into Hbar(1) / Hbar(0). Similarly, float('nan') and float('inf') I think it will be great if handle these in Hbar class: if isinstance(amount, bool) or (isinstance(amount, float) and not math.isfinite(amount)):
raise ValueError(f"Invalid Hbar amount: {amount}")Whats your thought? |
|
@manishdait Since |
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
1f4667b to
94186e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (15)
src/hiero_sdk_python/query/query.py (1)
157-161: Simplify error message formatting.The error message format uses both the symbol (ℏ) and the word (HBAR), which is redundant. Consider using
Hbar.__str__()for consistency.🔧 Proposed fix
if self.payment_amount > max_payment: raise ValueError( - f"Query cost ℏ{self.payment_amount.to_hbars()} HBAR " - f"exceeds max set query payment: ℏ{max_payment.to_hbars()} HBAR" + f"Query cost {self.payment_amount} " + f"exceeds max set query payment: {max_payment}" )CHANGELOG.md (1)
98-98: Consider splitting changelog entry for clarity.The single entry combines two distinct API additions. Splitting into separate bullets would improve readability and consistency with other entries.
📝 Suggested rewrite
-- Support for setting `max_query_payment`, `Query.set_max_query_payment()` allows setting a per-query maximum Hbar payment and `Client.set_default_max_query_payment()` sets a client-wide default maximum payment. +- Added per-query maximum payment via `Query.set_max_query_payment()`. +- Added client-wide default maximum query payment via `Client.set_default_max_query_payment()`.examples/contract/contract_execute_transaction.py (1)
110-121: Consider avoiding the double network call by setting query payment directly.The current pattern calls
get_cost()(which performs a COST_ANSWER query) and thenexecute()(which internally may perform another COST_ANSWER whenpayment_amountisNone). To avoid redundant network calls and potential flakiness if cost changes between calls, consider also settingset_query_payment(cost):Suggested improvement
cost = query.get_cost(client) + query.set_query_payment(cost) query.set_max_query_payment(cost) result = query.execute(client)Alternatively, for a simpler example that better demonstrates the protective feature, set a reasonable fixed maximum without fetching cost first.
examples/contract/ethereum_transaction.py (1)
149-160: Same double network call concern applies here.This pattern has the same issue as noted in past reviews:
get_cost()performs a COST_ANSWER, thenexecute()may perform another one internally. Settingmax_query_paymentto exactly the fetched cost also doesn't demonstrate the protective value of the feature.Suggested improvement
cost = query.get_cost(client) + query.set_query_payment(cost) # Reuse fetched cost to avoid second COST_ANSWER query.set_max_query_payment(cost) result = query.execute(client)Or simplify by using a fixed reasonable cap like
Hbar(2)without the cost-fetch step.examples/contract/contract_call_query.py (1)
117-128: Avoid double COST_ANSWER by also setting query payment.Per past review feedback: the explicit
get_cost()followed byexecute()likely triggers two COST_ANSWER queries sincepayment_amountremainsNone. Set the query payment to reuse the fetched cost.Suggested fix
cost = query.get_cost(client) + query.set_query_payment(cost) query.set_max_query_payment(cost) result = query.execute(client)tests/unit/client_test.py (3)
191-207: Fix parametrize syntax and add fluent interface assertion.Static analysis flags PT006: the first argument to
pytest.mark.parametrizeshould be a tuple when using multiple parameters. Additionally, per coding guidelines (PRIORITY 2), fluent setters should be tested to returnself, and clients should be closed for resource cleanup.Proposed fix
`@pytest.mark.parametrize`( - 'valid_amount,expected', + ('valid_amount', 'expected'), [ (1, Hbar(1)), (0.1, Hbar(0.1)), (Decimal('0.1'), Hbar(Decimal('0.1'))), (Hbar(1), Hbar(1)), (Hbar(0), Hbar(0)) ] ) def test_set_default_max_query_payment_valid_param(valid_amount, expected): """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" client = Client.for_testnet() - # by default is 1 hbar before setting it - assert client.default_max_query_payment == Hbar(1) - client.set_default_max_query_payment(valid_amount) - assert client.default_max_query_payment == expected + try: + assert client.default_max_query_payment == Hbar(1) + result = client.set_default_max_query_payment(valid_amount) + assert result is client, "set_default_max_query_payment should return self" + assert client.default_max_query_payment == expected + finally: + client.close()
209-218: Add resource cleanup for negative value tests.The client should be closed after test completion for proper resource management.
Proposed fix
`@pytest.mark.parametrize`( - 'negative_amount', + ('negative_amount',), [-1, -0.1, Decimal('-0.1'), Decimal('-1'), Hbar(-1)] ) def test_set_default_max_query_payment_negative_value(negative_amount): """Test set_default_max_query_payment for negative amount values.""" client = Client.for_testnet() - - with pytest.raises(ValueError, match="max_query_payment must be non-negative"): - client.set_default_max_query_payment(negative_amount) + try: + with pytest.raises(ValueError, match="max_query_payment must be non-negative"): + client.set_default_max_query_payment(negative_amount) + finally: + client.close()
220-232: Add resource cleanup for invalid param tests.Same cleanup consideration applies here.
Proposed fix
`@pytest.mark.parametrize`( - 'invalid_amount', + ('invalid_amount',), ['1', 'abc', True, False, None, object()] ) def test_set_default_max_query_payment_invalid_param(invalid_amount): """Test that set_default_max_query_payment raise error for invalid param.""" client = Client.for_testnet() - - with pytest.raises(TypeError, match=( - "max_query_payment must be int, float, Decimal, or Hbar, " - f"got {type(invalid_amount).__name__}" - )): - client.set_default_max_query_payment(invalid_amount) + try: + with pytest.raises(TypeError, match=( + "max_query_payment must be int, float, Decimal, or Hbar, " + f"got {type(invalid_amount).__name__}" + )): + client.set_default_max_query_payment(invalid_amount) + finally: + client.close()tests/integration/contract_call_query_e2e_test.py (2)
66-77: Useset_query_payment(cost)to avoid double cost fetch.The pattern here differs from
test_integration_contract_call_query_get_cost(Line 175) which correctly usesset_query_payment(cost)afterget_cost(). For consistency and to avoid double COST_ANSWER queries, apply the same pattern here:Suggested fix
cost = query.get_cost(env.client) + query.set_query_payment(cost) query.set_max_query_payment(cost) result = query.execute(env.client)Alternatively, for stable integration tests, consider setting a fixed cap like
Hbar(2)instead of the exact fetched cost.
116-127: Same fix needed here for consistency.Apply the same pattern as the previous test to avoid double cost queries:
Suggested fix
cost = query.get_cost(env.client) + query.set_query_payment(cost) query.set_max_query_payment(cost) result = query.execute(env.client)tests/unit/query_test.py (4)
302-321: LGTM! Minor typo in comment.The test correctly validates that a per-query
max_query_paymentcan override the client's default. Fix the typo on line 313: "paymnet" → "payment".
349-368: LGTM! Minor typo in comment.The test correctly validates that when no per-query max is set, the client's
default_max_query_paymentis used as the fallback. Fix the typo on line 360: "paymnet" → "payment".
251-253: Use direct truth check instead of equality comparison.Per Python best practices (E712), prefer direct truth checks over
== True.♻️ Proposed fix
assert query._is_payment_required() == True # Verify that payment-requiring query also defaults to requiring payment - assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()
265-270: Add assertion for fluent interface (method chaining).Per unit test coding guidelines (PRIORITY 1): "Assert fluent setters return
self". The test verifies value conversion but doesn't validate thatset_max_query_paymentreturns the query instance for method chaining.♻️ Proposed fix
def test_set_max_query_payment_valid_param(query, valid_amount, expected): """Test that set_max_query_payment correctly converts various input types to Hbar.""" # by default is none before setting it assert query.max_query_payment is None - query.set_max_query_payment(valid_amount) + result = query.set_max_query_payment(valid_amount) + assert result is query, "set_max_query_payment should return self for method chaining" assert query.max_query_payment == expectedtests/integration/query_e2e_test.py (1)
242-259: Improve assertion specificity and break down compound assertion.Per integration test coding guidelines (CRITICAL PRINCIPLES), tests must fail loudly and be diagnosable:
- Line 255:
pytest.raises(ValueError)is too broad. Add amatchparameter for specificity.- Line 259: The compound assertion (
and) makes it unclear which part failed if the test breaks.♻️ Proposed fix
- with pytest.raises(ValueError) as e: + with pytest.raises(ValueError, match=r"Query cost .* exceeds max set query payment:") as e: query.execute(env.client) msg = str(e.value) - assert "Query cost" in msg and "exceeds max set query payment:" in msg + assert "Query cost" in msg, "Error message should mention 'Query cost'" + assert "exceeds max set query payment:" in msg, "Error message should mention 'exceeds max set query payment:'" + assert "HBAR" in msg, "Error message should include 'HBAR'"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdexamples/contract/contract_call_query.pyexamples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pysrc/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/hbar.pysrc/hiero_sdk_python/query/query.pytests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.pytests/unit/client_test.pytests/unit/hbar_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (4)
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/contract/contract_execute_transaction.pyexamples/contract/ethereum_transaction.pyexamples/contract/contract_call_query.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.pytests/unit/hbar_test.py
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/contract_call_query_e2e_test.pytests/integration/query_e2e_test.py
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:28:14.391Z
Learnt from: github-actions[bot]
Repo: hiero-ledger/hiero-sdk-python PR: 0
File: :0-0
Timestamp: 2026-01-08T10:28:14.391Z
Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
Applied to files:
CHANGELOG.mdtests/unit/client_test.py
🧬 Code graph analysis (9)
examples/contract/contract_execute_transaction.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(285-325)set_max_query_payment(96-128)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(19-218)src/hiero_sdk_python/client/client.py (2)
Client(32-291)set_default_max_query_payment(249-278)
examples/contract/ethereum_transaction.py (1)
src/hiero_sdk_python/query/query.py (2)
get_cost(285-325)set_max_query_payment(96-128)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-128)_is_payment_required(418-425)get_cost(285-325)_before_execute(130-161)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-278)
tests/integration/contract_call_query_e2e_test.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (5)
ContractCallQuery(27-233)set_contract_id(59-67)set_gas(69-77)set_function(107-124)execute(190-216)src/hiero_sdk_python/query/query.py (2)
get_cost(285-325)set_max_query_payment(96-128)
examples/contract/contract_call_query.py (2)
src/hiero_sdk_python/contract/contract_call_query.py (1)
ContractCallQuery(27-233)src/hiero_sdk_python/query/query.py (2)
get_cost(285-325)set_max_query_payment(96-128)
tests/unit/hbar_test.py (1)
src/hiero_sdk_python/hbar.py (3)
Hbar(19-218)to_tinybars(68-70)from_tinybars(102-114)
tests/integration/query_e2e_test.py (3)
src/hiero_sdk_python/query/account_info_query.py (1)
AccountInfoQuery(11-131)tests/integration/utils.py (4)
IntegrationTestEnv(34-96)env(23-27)close(51-52)create_account(54-67)src/hiero_sdk_python/query/query.py (1)
set_max_query_payment(96-128)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(19-218)to_hbars(72-76)
🪛 GitHub Check: Codacy Static Code Analysis
tests/unit/query_test.py
[warning] 297-297: tests/unit/query_test.py#L297
function already defined line 285
🪛 Ruff (0.14.11)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/unit/query_test.py
253-253: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
297-297: Redefinition of unused test_set_max_query_payment_invalid_param from line 285: test_set_max_query_payment_invalid_param redefined here
(F811)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-266: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/hbar_test.py
229-229: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
src/hiero_sdk_python/hbar.py
44-44: Avoid specifying long messages outside the exception class
(TRY003)
44-44: f-string without any placeholders
Remove extraneous f prefix
(F541)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
47-47: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/integration/query_e2e_test.py
243-243: Redefinition of unused env from line 12: env redefined here
(F811)
255-255: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
259-259: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-116: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
158-161: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (14)
src/hiero_sdk_python/hbar.py (1)
9-9: Input validation enhancements look correct.The added validation properly:
- Rejects
boolvalues (sinceboolis a subclass ofint)- Rejects non-finite floats (
inf,nan) usingmath.isfinite- Applies consistent bool rejection in
from_tinybarsThis aligns with the PR discussion to enforce these invariants inside the
Hbarclass.Also applies to: 43-47, 112-113
src/hiero_sdk_python/query/query.py (3)
60-60: New public attributemax_query_paymentcorrectly added.The attribute is properly typed as
Optional[Hbar]and initialized toNone, allowing per-query overrides while falling back to the client default.
96-128: Validation inset_max_query_paymentis comprehensive.The method correctly:
- Rejects
boolvalues explicitly (sinceboolis a subclass ofint)- Validates accepted types (int, float, Decimal, Hbar)
- Normalizes to
Hbarwhen needed- Enforces non-negativity
- Returns
selffor fluent chainingThis addresses the concerns raised in past reviews about bool validation.
147-161: Max payment validation correctly placed in execution flow.The validation occurs in
_before_executeafter fetching the cost but before attaching payment, which is the correct location per Query semantics. The fallback toclient.default_max_query_paymentwhenself.max_query_paymentisNonealigns with the PR objectives.tests/unit/hbar_test.py (1)
24-41: Good test coverage for constructor validation.The parametrized tests comprehensively cover:
- Invalid types: string, bool (True/False), dict, class object
- Non-finite floats:
infandnanThese tests will fail loudly with descriptive messages if the validation changes.
src/hiero_sdk_python/client/client.py (2)
23-24: Client-wide default max query payment correctly implemented.The constant
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a sensible default (1 HBAR), and the instance attribute is properly initialized in__init__.Also applies to: 52-52
249-278: Validation inset_default_max_query_paymentis comprehensive and consistent.The method correctly:
- Rejects
boolvalues explicitly (addressing past review feedback)- Validates accepted types (int, float, Decimal, Hbar)
- Normalizes to
Hbarwhen needed- Enforces non-negativity for both numeric and Hbar inputs
- Returns
selffor fluent chainingThe implementation is consistent with
Query.set_max_query_payment.tests/integration/contract_call_query_e2e_test.py (1)
163-178: Good implementation of the cost-aware pattern.This test correctly demonstrates the intended usage: fetch cost once with
get_cost(), then set the exact payment withset_query_payment(cost)to avoid a second network call duringexecute(). This is the pattern that should be replicated in the other tests.tests/unit/query_test.py (5)
1-2: LGTM!Imports and
pytestmarkare correctly set up for the unit tests.Also applies to: 15-16
62-62: LGTM!Correctly sets
max_query_paymenthigher than the mocked cost (2 Hbar) to allow execution to proceed without hitting the max payment validation.
272-279: LGTM!Good coverage of negative value edge cases across different types (int, float, Decimal, Hbar).
323-346: LGTM!Correctly tests that the per-query
max_query_paymenttakes precedence over the client default when determining if the cost exceeds the limit. Good use ofre.escapefor the error message containing the ℏ symbol.
370-384: LGTM!Completes the test coverage matrix by validating the error path when the query cost exceeds the client's default max payment (with no per-query override set).
tests/integration/query_e2e_test.py (1)
9-12: LGTM!Imports are correctly added for the new integration test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
94186e3 to
ba9f4b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tests/unit/client_test.py (4)
191-207: Missing fluent interface assertion and resource cleanup.Per coding guidelines (PRIORITY 1 & 2), fluent setters should be tested to return
selffor method chaining. Additionally, the client should be closed for resource cleanup.♻️ Proposed fix
def test_set_default_max_query_payment_valid_param(valid_amount, expected): """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" client = Client.for_testnet() - # by default is 1 hbar before setting it - assert client.default_max_query_payment == Hbar(1) - client.set_default_max_query_payment(valid_amount) - assert client.default_max_query_payment == expected + try: + # by default is 1 hbar before setting it + assert client.default_max_query_payment == Hbar(1) + result = client.set_default_max_query_payment(valid_amount) + assert result is client, "set_default_max_query_payment should return self for method chaining" + assert client.default_max_query_payment == expected + finally: + client.close()
209-218: Missing resource cleanup.The test creates a client but doesn't close it after the test completes.
♻️ Proposed fix
def test_set_default_max_query_payment_negative_value(negative_amount): """Test set_default_max_query_payment for negative amount values.""" client = Client.for_testnet() - - with pytest.raises(ValueError, match="max_query_payment must be non-negative"): - client.set_default_max_query_payment(negative_amount) + try: + with pytest.raises(ValueError, match="max_query_payment must be non-negative"): + client.set_default_max_query_payment(negative_amount) + finally: + client.close()
220-232: Missing resource cleanup.Same cleanup consideration as other tests - close the client after the test.
♻️ Proposed fix
def test_set_default_max_query_payment_invalid_param(invalid_amount): """Test that set_default_max_query_payment raise error for invalid param.""" client = Client.for_testnet() - - with pytest.raises(TypeError, match=( - "max_query_payment must be int, float, Decimal, or Hbar, " - f"got {type(invalid_amount).__name__}" - )): - client.set_default_max_query_payment(invalid_amount) + try: + with pytest.raises(TypeError, match=( + "max_query_payment must be int, float, Decimal, or Hbar, " + f"got {type(invalid_amount).__name__}" + )): + client.set_default_max_query_payment(invalid_amount) + finally: + client.close()
234-243: Missing resource cleanup.Consistent with other tests, close the client after the test.
♻️ Proposed fix
def test_set_default_max_query_payment_non_finite_value(invalid_amount): """Test that set_default_max_query_payment raise error for non finite value.""" client = Client.for_testnet() - - with pytest.raises(ValueError, match="Hbar amount must be finite"): - client.set_default_max_query_payment(invalid_amount) + try: + with pytest.raises(ValueError, match="Hbar amount must be finite"): + client.set_default_max_query_payment(invalid_amount) + finally: + client.close()tests/unit/query_test.py (2)
265-270: Missing fluent interface assertion.Per coding guidelines (PRIORITY 2): "Assert fluent setters return
self". The test verifies value conversion but doesn't validate thatset_max_query_paymentreturns the query instance for method chaining.♻️ Proposed fix
def test_set_max_query_payment_valid_param(query, valid_amount, expected): """Test that set_max_query_payment correctly converts various input types to Hbar.""" # by default is none before setting it assert query.max_query_payment is None - query.set_max_query_payment(valid_amount) + result = query.set_max_query_payment(valid_amount) + assert result is query, "set_max_query_payment should return self for method chaining" assert query.max_query_payment == expected
251-253: Prefer direct truth check instead of equality comparison toTrue.Per Python best practices (Ruff E712), use direct truth check for boolean returns.
♻️ Proposed fix
def test_query_payment_requirement_defaults_to_true(query_requires_payment): """Test that the base Query class and payment-requiring queries default to requiring payment.""" query = Query() - assert query._is_payment_required() == True + assert query._is_payment_required() # Verify that payment-requiring query also defaults to requiring payment - assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/query/query.pytests/unit/client_test.pytests/unit/query_test.pytests/unit/topic_info_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/query_test.pytests/unit/topic_info_test.pytests/unit/client_test.py
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:28:14.391Z
Learnt from: github-actions[bot]
Repo: hiero-ledger/hiero-sdk-python PR: 0
File: :0-0
Timestamp: 2026-01-08T10:28:14.391Z
Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
Applied to files:
tests/unit/client_test.py
🧬 Code graph analysis (3)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(19-218)to_hbars(72-76)
tests/unit/client_test.py (2)
src/hiero_sdk_python/hbar.py (1)
Hbar(19-218)src/hiero_sdk_python/client/client.py (4)
Client(32-291)from_env(59-103)for_testnet(106-115)set_default_max_query_payment(249-278)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(19-218)
🪛 GitHub Actions: Code Coverage
tests/unit/topic_info_test.py
[error] 256-256: NameError: name 'repr_outputgit' is not defined in test_repr_and_str. Likely a typo; expected 'repr_output'.
🪛 Ruff (0.14.11)
tests/unit/query_test.py
253-253: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-116: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
158-161: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/topic_info_test.py
256-256: Undefined name repr_outputgit
(F821)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-266: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (12)
src/hiero_sdk_python/query/query.py (4)
5-7: LGTM!Import changes are appropriate:
Decimaladded for the new numeric input support inset_max_query_payment, andListremoved as it's no longer used in this file.
60-60: LGTM!New public attribute
max_query_paymentfollows the existing pattern for optionalHbarfields (similar topayment_amount). Default ofNoneis appropriate since the client-level default serves as fallback.
96-128: LGTM! Validation logic is complete and correctly handles edge cases.The implementation properly:
- Rejects
boolbefore theisinstancecheck (sinceboolis a subclass ofint)- Validates type is one of
int,float,Decimal, orHbar- Preserves
Hbarinputs, converts others viaHbar()constructor (which handles non-finite float rejection persrc/hiero_sdk_python/hbar.pylines 45-46)- Enforces non-negativity on the final
Hbarvalue- Returns
selffor fluent chainingThis aligns with the
Hbarconstructor's own validation (see relevant code snippet showingbooland non-finite checks inHbar.__init__).
149-161: Payment safety preserved; max payment check correctly scoped to paid queries.The max payment validation is correctly placed:
- Only triggers when
payment_amountwasNoneAND_is_payment_required()returnsTrue- Uses per-query
max_query_paymentwith client default as fallback- Raises before any payment transaction is built
This maintains the FREE vs PAID query classification and doesn't attach payment to COST_ANSWER queries (per REVIEW FOCUS 1 in coding guidelines).
One minor note: the error message uses both the symbol (ℏ) and the word (HBAR), which is slightly redundant but doesn't affect functionality.
src/hiero_sdk_python/client/client.py (4)
5-5: LGTM!Import additions for
DecimalandHbarare necessary for the newset_default_max_query_paymentmethod.Also applies to: 11-11
23-24: LGTM!Module-level constant
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a sensible default (1 HBAR) and is publicly accessible for documentation/introspection purposes.
52-52: LGTM!Instance attribute initialized from the module constant ensures consistent defaults across all
Clientinstances.
249-278: LGTM! Implementation is consistent with Query.set_max_query_payment().The validation logic correctly:
- Rejects
boolexplicitly before theisinstancecheck- Validates acceptable types (
int,float,Decimal,Hbar)- Preserves
Hbarinputs, converts others viaHbar()constructor- Enforces non-negativity on the final
Hbarvalue- Returns
selffor fluent method chainingThe
Hbarconstructor handles non-finite floats (inf,nan), so explicit checks here are not needed.tests/unit/query_test.py (4)
1-2: LGTM!Appropriate imports added for
Decimal(testing numeric input types) andre(forre.escapein error message matching). Thepytestmark = pytest.mark.unitproperly categorizes these as unit tests.Also applies to: 15-16
62-62: Good addition to maintain test validity with new max payment check.Setting
max_query_paymenttoHbar(3)ensures the mocked cost ofHbar(2)doesn't exceed the limit, allowing the existing test to continue validating payment setup behavior.
272-300: LGTM! Good coverage of error paths.Tests correctly validate:
- Negative values (including negative
Hbarinstances)- Invalid types (
str,bool,None,object)- Non-finite floats (
inf,nan)The error message matching is appropriate for each case.
302-384: LGTM! Comprehensive client/query interaction tests.The tests thoroughly validate:
- Per-query
max_query_paymentoverrides client default- Execution succeeds when cost ≤ per-query maximum
- Execution fails with correct error when cost > per-query maximum
- Client
default_max_query_paymentis used as fallback- Execution fails with correct error when cost > client default
Using
re.escape()for error message matching correctly handles the special characters (ℏ) in the message.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Manish Dait <[email protected]>
ba9f4b7 to
4e05b6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/hiero_sdk_python/hbar.py (1)
43-47: LGTM! Input validation correctly rejects booleans and non-finite floats.The validation order is correct: checking
isinstance(amount, bool)before the(int, float, Decimal)check properly handles Python'sboolbeing a subclass ofint. Themath.isfinite()check correctly rejectsinf,-inf, andnan.Note: The f-string prefixes on lines 44 and 47 are unnecessary (no placeholders), but this was already flagged in a previous review.
tests/unit/hbar_test.py (1)
24-31: Tests correctly validate invalid type rejection.The parametrized test covers the key invalid types. Note: Line 26 uses
object(the class) rather thanobject()(an instance), which was flagged in a previous review.tests/unit/client_test.py (4)
191-207: Add fluent interface assertion and resource cleanup.Per coding guidelines (PRIORITY 1), fluent setters must be tested to return
self. Additionally, the client should be closed for proper resource management. The static analysis hint about tuple syntax forparametrizeis also valid.🔧 Proposed fix
`@pytest.mark.parametrize`( - 'valid_amount,expected', + ('valid_amount', 'expected'), [ (1, Hbar(1)), (0.1, Hbar(0.1)), (Decimal('0.1'), Hbar(Decimal('0.1'))), (Hbar(1), Hbar(1)), (Hbar(0), Hbar(0)) ] ) def test_set_default_max_query_payment_valid_param(valid_amount, expected): """Test that set_default_max_query_payment correctly converts various input types to Hbar.""" client = Client.for_testnet() # by default is 1 hbar before setting it assert client.default_max_query_payment == Hbar(1) - client.set_default_max_query_payment(valid_amount) + result = client.set_default_max_query_payment(valid_amount) + assert result is client, "set_default_max_query_payment should return self for method chaining" assert client.default_max_query_payment == expected + client.close()
209-218: Add resource cleanup for negative value tests.The test creates a client but doesn't close it. Add
client.close()after the assertion block for consistency with other tests in this file.
220-232: Add resource cleanup for invalid param tests.Same cleanup consideration as the negative value tests.
234-243: Add resource cleanup for non-finite value tests.The test should close the client after the assertion.
tests/unit/query_test.py (4)
248-253: Use direct truth check instead of equality comparison toTrue.Per Python best practices (E712), use direct truth check for boolean returns on both lines 251 and 253.
♻️ Proposed fix
def test_query_payment_requirement_defaults_to_true(query_requires_payment): """Test that the base Query class and payment-requiring queries default to requiring payment.""" query = Query() - assert query._is_payment_required() == True + assert query._is_payment_required() # Verify that payment-requiring query also defaults to requiring payment - assert query_requires_payment._is_payment_required() == True + assert query_requires_payment._is_payment_required()
255-270: Add fluent setter assertion and use tuple for parametrize.Per coding guidelines (PRIORITY 1): "Assert fluent setters return
self". The test verifies value conversion but doesn't validate method chaining.Additionally, PT006 indicates the parametrize argnames should use a tuple.
♻️ Proposed fix
`@pytest.mark.parametrize`( - 'valid_amount,expected', + ("valid_amount", "expected"), [ (1, Hbar(1)), (0.1, Hbar(0.1)), (Decimal('0.1'), Hbar(Decimal('0.1'))), (Hbar(1), Hbar(1)), (Hbar(0), Hbar(0)) ] ) def test_set_max_query_payment_valid_param(query, valid_amount, expected): """Test that set_max_query_payment correctly converts various input types to Hbar.""" # by default is none before setting it assert query.max_query_payment is None - query.set_max_query_payment(valid_amount) + result = query.set_max_query_payment(valid_amount) + assert result is query, "set_max_query_payment should return self for method chaining" assert query.max_query_payment == expected
302-321: Fix typo in comment.Line 313: "paymnet" should be "payment".
♻️ Proposed fix
- # mock the get_cost to return 2 hbar as required paymnet + # mock the get_cost to return 2 hbar as required payment
349-368: Fix typo in comment.Line 360: "paymnet" should be "payment".
♻️ Proposed fix
- # mock the get_cost to return 2 hbar as required paymnet + # mock the get_cost to return 2 hbar as required payment
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/hiero_sdk_python/client/client.pysrc/hiero_sdk_python/hbar.pysrc/hiero_sdk_python/query/query.pytests/unit/client_test.pytests/unit/hbar_test.pytests/unit/query_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/client_test.pytests/unit/query_test.pytests/unit/hbar_test.py
src/hiero_sdk_python/query/**/*.py
⚙️ CodeRabbit configuration file
src/hiero_sdk_python/query/**/*.py: You are acting as a senior maintainer reviewing the Query base class
and its subclasses for the hiero-sdk-python project.NOTE:
- Review focus levels indicate areas that are important to check carefully.
- They do NOT imply severity or urgency.
- Only recommend fixes when there is a clear behavioral regression.
Scope is STRICTLY LIMITED to:
- Changes to the base
Queryclass- Changes to existing
Querysubclasses- Newly added
Querysubclasses
REVIEW FOCUS 1 — QUERY SEMANTICS & PAYMENT BEHAVIOR
(CONTRACTUAL / HIGH SENSITIVITY)Queries do not reach consensus and use
QueryHeaderfor payment and responseType.The following behaviors are contractual and must remain unchanged:
_is_payment_required()semantics- FREE vs PAID query classification
- COST_ANSWER vs ANSWER_ONLY behavior
- Whether a payment transaction is attached
Good to check and verify that changes do NOT:
- Alter FREE → PAID or PAID → FREE behavior
- Attach payment to COST_ANSWER queries
- Bypass
get_cost(client)for paid queries- Hardcode fees or override payment logic
REVIEW FOCUS 2 — EXECUTION LIFECYCLE & BASE CLASS INTEGRITY
All queries MUST:
- Use the base
Queryexecution flow- Delegate retries, backoff, and node selection to
_Executable- Call
_before_execute(client)before_execute(client)Subclasses MUST NOT:
- Override retry logic
- Implement custom node selection
- Manage gRPC deadlines manually
- Bypass
_Executablestate handlingFlag deviations for review; recommend fixes only if behavior changes.
REVIEW FOCUS 3 — REQUEST CONSTRUCTION CONTRACT
_make_request()MUST:
- Validat...
Files:
src/hiero_sdk_python/query/query.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:28:14.391Z
Learnt from: github-actions[bot]
Repo: hiero-ledger/hiero-sdk-python PR: 0
File: :0-0
Timestamp: 2026-01-08T10:28:14.391Z
Learning: In hiero-ledger/hiero-sdk-python, examples should prefer Client.from_env() (src/hiero_sdk_python/client/client.py) for client setup. For examples under examples/tokens, replace manual Network/AccountId/PrivateKey + load_dotenv() bootstrapping with Client.from_env(), and use client.operator_account_id and client.operator_private_key instead of passing operator_id/operator_key through functions.
Applied to files:
tests/unit/client_test.py
🧬 Code graph analysis (5)
tests/unit/query_test.py (2)
src/hiero_sdk_python/query/query.py (4)
set_max_query_payment(96-128)_is_payment_required(418-425)get_cost(285-325)_before_execute(130-161)src/hiero_sdk_python/client/client.py (1)
set_default_max_query_payment(249-278)
src/hiero_sdk_python/hbar.py (1)
tests/unit/conftest.py (1)
amount(39-41)
tests/unit/hbar_test.py (1)
src/hiero_sdk_python/hbar.py (4)
Hbar(19-218)from_gigabars(157-162)to_tinybars(68-70)from_tinybars(102-114)
src/hiero_sdk_python/query/query.py (1)
src/hiero_sdk_python/hbar.py (2)
Hbar(19-218)to_hbars(72-76)
src/hiero_sdk_python/client/client.py (1)
src/hiero_sdk_python/hbar.py (1)
Hbar(19-218)
🪛 Ruff (0.14.11)
tests/unit/client_test.py
192-192: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
tests/unit/query_test.py
253-253: Avoid equality comparisons to True; use query_requires_payment._is_payment_required(): for truth checks
Replace with query_requires_payment._is_payment_required()
(E712)
256-256: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
src/hiero_sdk_python/hbar.py
44-44: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/query/query.py
96-96: Use X | Y for type annotations
Convert to X | Y
(UP007)
113-116: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
158-161: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/client/client.py
7-7: typing.List is deprecated, use list instead
(UP035)
249-249: Use X | Y for type annotations
Convert to X | Y
(UP007)
263-266: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (12)
src/hiero_sdk_python/hbar.py (1)
112-114: LGTM!from_tinybarsvalidation is consistent with constructor.The boolean check pattern mirrors the constructor validation, ensuring consistent behavior across all entry points.
tests/unit/hbar_test.py (1)
223-230: LGTM! Test correctly usesre.escapefor regex pattern.The test properly escapes the period in the error message, addressing the regex metacharacter concern from a previous review.
src/hiero_sdk_python/client/client.py (2)
23-24: LGTM! Well-placed module-level constant for default max query payment.
DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)provides a clear, documented default that aligns with the PR objectives.
249-278: LGTM!set_default_max_query_paymentimplementation is robust.The method correctly:
- Rejects booleans before checking numeric types (Line 262)
- Preserves existing
Hbarinstances without re-wrapping (Lines 268-272)- Validates non-negativity against
Hbar(0)using proper comparison (Line 274)- Returns
selffor method chaining (Line 278)The validation is now consistent with
Hbar.__init__which handles non-finite float rejection internally.src/hiero_sdk_python/query/query.py (3)
60-60: LGTM!max_query_paymentattribute properly added.The optional attribute defaults to
None, allowing per-query overrides while falling back to client defaults.
96-128: LGTM!set_max_query_paymentvalidation is consistent with Client.The validation pattern correctly:
- Rejects booleans before type checking (Line 112)
- Preserves
Hbarinstances, converts others (Lines 118-122)- Validates non-negativity (Lines 124-125)
- Returns
selffor fluent chaining (Line 128)This maintains consistency with
Client.set_default_max_query_payment().
149-161: LGTM! Payment validation logic correctly integrates with query lifecycle.The implementation correctly:
- Only validates when
payment_amountwas not explicitly set (Line 147)- Falls back to
client.default_max_query_paymentwhen query-level max is not set (Lines 151-155)- Raises early before submitting the query if cost exceeds max (Lines 157-161)
This aligns with REVIEW FOCUS 1 (payment semantics) and REVIEW FOCUS 2 (execution lifecycle) from the coding guidelines - the check happens in
_before_executeafter fetching cost but before actual query submission.Minor: The error message format uses both
ℏsymbol andHBARtext redundantly, which was flagged in a previous review. Consider usingHbar.__str__()for cleaner formatting.tests/unit/query_test.py (5)
1-15: LGTM!Imports are appropriate for the test coverage. The
pytestmark = pytest.mark.unitcorrectly marks all tests in this module.
56-78: LGTM!The addition of
set_max_query_payment(Hbar(3))is necessary to ensure the test passes with the new cost validation logic. The max of 3 Hbar correctly exceeds the mocked cost of 2 Hbar, allowing the test to verify the payment setup behavior without triggering the new validation error.
293-300: LGTM!The duplicate function name issue has been resolved. Test correctly validates that non-finite floats (
inf,nan) raiseValueErrorwith the expected message.
323-346: LGTM!Well-structured test that verifies the query-specific max takes precedence over a higher client default. The use of
re.escape()correctly handles the ℏ symbol in the error message.
370-384: LGTM! Comprehensive client interaction tests.This test completes the test matrix for client/query max payment interactions:
- ✓ Query override succeeds (lines 302-321)
- ✓ Query override fails when exceeded (lines 323-346)
- ✓ Client fallback succeeds (lines 349-368)
- ✓ Client fallback fails when exceeded (lines 370-384)
The use of
re.escape()correctly handles special characters in the error message.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @pytest.mark.parametrize( | ||
| 'negative_amount', | ||
| [-1, -0.1, Decimal('-0.1'), Decimal('-1'), Hbar(-1), Hbar(-0.2)] | ||
| ) | ||
| def test_set_max_query_payment_negative_value(query, negative_amount): | ||
| """Test set_max_query_payment for negative amount values.""" | ||
| with pytest.raises(ValueError, match="max_query_payment must be non-negative"): | ||
| query.set_max_query_payment(negative_amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Good coverage for negative values; minor parametrize style issue.
The test correctly covers negative inputs across all supported types including Hbar(-1) and Hbar(-0.2). The simplified match pattern avoids regex issues.
Per PT006, consider using a tuple for the parametrize argument: ("negative_amount",).
| @pytest.mark.parametrize( | ||
| 'invalid_amount', | ||
| ['1', 'abc', True, False, None, object()] | ||
| ) | ||
| def test_set_max_query_payment_invalid_param(query, invalid_amount): | ||
| """Test that set_max_query_payment raise error for invalid param.""" | ||
| with pytest.raises(TypeError, match=( | ||
| "max_query_payment must be int, float, Decimal, or Hbar, " | ||
| f"got {type(invalid_amount).__name__}" | ||
| )): | ||
| query.set_max_query_payment(invalid_amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Good coverage for invalid types including booleans.
The test correctly includes True and False per the PR discussion about rejecting boolean inputs. The dynamic match message construction is appropriate.
Per PT006, consider using a tuple: ("invalid_amount",).
AntonioCeppellini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! seems ready to go
aceppaluni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work!!
aceppaluni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Super great work!!
Description:
This PR introduces support for maximum query payment limits for
Queryclass by:max_query_payment fieldto the Query base class.set_max_query_payment(...)max_query_payment, if set, ordefault_max_query_paymentas a fallbackChanges Made:
max_query_paymentsupport to the Query base class.set_max_query_payment()to allow setting a per-query maximum payment.DEFAULT_MAX_QUERY_PAYMENT = Hbar(1)to client.set_default_max_query_payment()on Client to define a client-wide default maximum query payment.Related issue(s):
Fixes #1346
Notes for reviewer:
Checklist